Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

linker: Avoid library duplication with /WHOLEARCHIVE #84866

Merged
merged 1 commit into from
May 7, 2021

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented May 3, 2021

Looks like in #72785 I misinterpreted how the link.exe's /WHOLEARCHIVE flag works.

It's not necessary to write mylib /WHOLEARCHIVE:mylib to mark mylib as whole archive, /WHOLEARCHIVE:mylib alone is enough.

https://docs.microsoft.com/en-us/cpp/build/reference/wholearchive-include-all-library-object-files?view=msvc-160

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 3, 2021
@Mark-Simulacrum
Copy link
Member

It's worth noting that the current behavior implied whole archive for all libraries passed, right? So this could regress code in theory? It seems OK to have the more limited and arguably correct behavior, though.

@bors r+

@bors
Copy link
Contributor

bors commented May 6, 2021

📌 Commit 71c509b894dc67b152522d756ce3e9216cc95eef has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 6, 2021
@petrochenkov
Copy link
Contributor Author

It's worth noting that the current behavior implied whole archive for all libraries passed, right? So this could regress code in theory?

No?
/WHOLEARCHIVE:foo affects only foo.

In GNU syntax the previous behavior was -lfoo --whole-archive -lfoo --no-whole-archive, this PR changes it to simply --whole-archive -lfoo --no-whole-archive, this shouldn't be a change in behavior.

@Mark-Simulacrum
Copy link
Member

Oh, I misread the PR description slightly - I thought we were currently emitting /WHOLEARCHIVE mylib rather than /WHOLEARCHIVE:mylib mylib. No problem then.

@bors

This comment has been minimized.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 6, 2021
@petrochenkov
Copy link
Contributor Author

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented May 6, 2021

📌 Commit fb9feb3 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 6, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 7, 2021
…lacrum

linker: Avoid library duplication with `/WHOLEARCHIVE`

Looks like in rust-lang#72785 I misinterpreted how the `link.exe`'s `/WHOLEARCHIVE` flag works.

It's not necessary to write `mylib /WHOLEARCHIVE:mylib` to mark `mylib` as whole archive, `/WHOLEARCHIVE:mylib` alone is enough.

https://docs.microsoft.com/en-us/cpp/build/reference/wholearchive-include-all-library-object-files?view=msvc-160
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 7, 2021
…lacrum

linker: Avoid library duplication with `/WHOLEARCHIVE`

Looks like in rust-lang#72785 I misinterpreted how the `link.exe`'s `/WHOLEARCHIVE` flag works.

It's not necessary to write `mylib /WHOLEARCHIVE:mylib` to mark `mylib` as whole archive, `/WHOLEARCHIVE:mylib` alone is enough.

https://docs.microsoft.com/en-us/cpp/build/reference/wholearchive-include-all-library-object-files?view=msvc-160
bors added a commit to rust-lang-ci/rust that referenced this pull request May 7, 2021
Rollup of 9 pull requests

Successful merges:

 - rust-lang#84254 (illumos should put libc last in library search order)
 - rust-lang#84442 (Unify rustc and rustdoc parsing of `cfg()`)
 - rust-lang#84655 (Cleanup of `wasm`)
 - rust-lang#84866 (linker: Avoid library duplication with `/WHOLEARCHIVE`)
 - rust-lang#84930 (rename LLVM target for RustyHermit)
 - rust-lang#84991 (rustc: Support Rust-specific features in -Ctarget-feature)
 - rust-lang#85029 (SGX mutex is movable)
 - rust-lang#85030 (Rearrange SGX split module files)
 - rust-lang#85033 (some further small cleanups)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6e4c842 into rust-lang:master May 7, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants